Skip to content

[PLAT-11587] Update Mac Arm64 version of node#2

Merged
juj merged 9 commits into
Unity-Technologies:3.1.38.3-unityfrom
rachelmsimm:master
Feb 27, 2025
Merged

[PLAT-11587] Update Mac Arm64 version of node#2
juj merged 9 commits into
Unity-Technologies:3.1.38.3-unityfrom
rachelmsimm:master

Conversation

@rachelmsimm

@rachelmsimm rachelmsimm commented Feb 14, 2025

Copy link
Copy Markdown
Collaborator

Description

This PR is for updating the version of node we're using for building emscripten for Mac Arm64. This change was needed because the Build Emsdk Mac (M1 Arm64) job was failing with the error that the version of node we were using wasn't compatible with our CPU. This is because Mac M1 is only supported by node versions 16 or higher. This PR updates the node version being used by the job to be 20.18.0, which is the most up to date node version being used by the upstream emsdk repository at the time of writing this PR. Also included in this PR is this change which was cherry-picked from the upstream emsdk repository. This was necessary to add because previously we had references to "arm64" and "aarch64" which represented the same architecture, but conflicted with each other when they shouldn't because we were using two separate names for the same thing.

Testing

  1. Passing yamato job: https://unity-ci.cds.internal.unity3d.com/job/47292045
  2. Passing Build All Emsdk suite to ensure no regressions to other OSes: https://unity-ci.cds.internal.unity3d.com/job/47382044/results
  3. Manually verified using the file command that the node, python, emscripten, binaryen, and llvm binaries generated as part of the artifact were for Mac Arm64

@rachelmsimm rachelmsimm changed the base branch from 3.1.38-unity to 3.1.38.3-unity February 14, 2025 23:00
@rachelmsimm rachelmsimm requested a review from juj February 25, 2025 21:15
@rachelmsimm rachelmsimm marked this pull request as ready for review February 25, 2025 21:16
Comment thread emsdk_manifest.json
{
"id": "node",
"version": "20.18.0",
"arch": "arm64",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of renaming everything from aarch64 to arm64, could this line have been renamed from arm64 to aarch64 instead? (so that no other changes but adding this one JSON entry here, and below for the SDK, would be needed?)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably, I just thought it might be a good opportunity to consolidate the references since we use both in different places

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cherry-picked the change from upstream emscripten by the way: see here

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be a good opportunity to consolidate

The consolidation would be best to happen when we update to upstream code. Ideally we would just add the minimum amount of code that would be specific to mac arm 64 into our downstream fork.

But in the interest of saving time, let's go with this form.

@irina-ishchenko irina-ishchenko self-requested a review February 26, 2025 17:41
@juj juj merged commit e6d1611 into Unity-Technologies:3.1.38.3-unity Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants